-
Notifications
You must be signed in to change notification settings - Fork 8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove support for time-based interval index patterns with migration #35262
Remove support for time-based interval index patterns with migration #35262
Conversation
Pinging @elastic/kibana-app-arch |
💔 Build Failed |
Based on the message, I understand that the users don't have to click on the I suggest we have a link in the message to the 7.0 breaking changes @gchaps can you review the text in the notification and the index pattern page? |
Here are some suggestions. Warning 1Support for time intervals was removed You are querying all indices that match logstash-*. For more information, view the index pattern in management. Note: Can you add a space after the last sentence and the index pattern name? Warning 2Support for time-based index patterns was removed These index patterns will not work in the next major version of Kibana. By clicking the Migrate button, you can migrate your index pattern to a wildcard pattern. Migrate Page textThis page lists every field in the index and its mapping type. To change a field type, use the Elasticsearch Mapping API. |
36ef2ca
to
53d6548
Compare
That's correct, Kibana will query the wildcard pattern instead of the interval-based pattern, so in your example it would be |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
7e1d940
to
a65217c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a pre-existing issue, but one thing I noticed is if I import an old interval based pattern via the management UI the interval name gets removed but the title is not migrated, so you end up with a pattern that does not work, but you also get no warnings (because the condition to show the warnings is based on whether the intervalName attribute exists).
Otherwise this LGTM. I did a fairly brief review of the code but nothing popped out at me. I tested the functionality with an old interval based pattern and everything I tried worked as expected.
this.title, this.getInterval(), start, stop, sortDirection | ||
); | ||
} | ||
getIndex() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some tests for this method? This regex magic is complicated enough I just want to make sure it handles every edge case we might expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM - tested & everything seems to work as intended. Added two minor UX notes on the toast.
const warningText = i18n.translate('common.ui.indexPattern.warningText', { | ||
defaultMessage: 'Instead of querying indices only matching the pattern {title}, you are now querying all indices ' + | ||
'matching {index}. This index pattern should be migrated to a wildcard-based index pattern. To do so, edit the ' + | ||
'index pattern in Management.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX nit: This message is hard to read before the notification disappears. I wonder if we could wordsmith it to make things shorter, e.g.
Support for time interval index patterns removed
Currently querying all indices matching {index}.
{title} should be migrated to a wildcard-based index pattern.
Edit >
<EuiFlexItem grow={false}> | ||
<EuiButton size="s" href={kbnUrl.getRouteHref(indexPattern, 'edit')}> | ||
Edit index pattern | ||
</EuiButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX nit: Maybe this is something to add later, but I wonder if we should be integrating with feature controls here. That way we could hide the edit button if we know a user doesn't have access to management, rather than allowing them to click a button that will lead straight to an error page.
💚 Build Succeeded |
💚 Build Succeeded |
cf4f550
to
82f085f
Compare
💚 Build Succeeded |
}).then(({ attributes: { title, intervalName } }) => { | ||
this.title = title; | ||
this.intervalName = intervalName; | ||
}).then(() => this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's a 'nip' from my side but I don't understand why we need it .then(() => this);
We can just return this
from the previous then
.
It may look a little worse, but we are not planning an additional Microtask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's totally true. I think I was just following the same style as above, but it could definitely be changed.
This is an intermediary step for 7.x to work towards #35173 without breaking existing users with time-based interval index patterns.
With this PR, when you load a time-based interval index pattern, you'll get a warning message:
The warning message explains that you are now querying the index, as converted to wildcards (which may include additional indices than the original index pattern). It also has a link to the index pattern in advanced settings, where you can actually easily migrate the index pattern: